Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refacto language + add navigator preferences #349

Merged
merged 10 commits into from
Nov 15, 2024
Merged

Refacto language + add navigator preferences #349

merged 10 commits into from
Nov 15, 2024

Conversation

louisgreiner
Copy link
Collaborator

@louisgreiner louisgreiner commented Nov 13, 2024 •
edited
Loading

Description

  • Initialize application language with navigator language if no preference (= first connection) in localStorage
  • Allow to change language from the application without window reload
  • Allow to change language from the outer of the application with @Input (setter) (in standalone mode especially)

Issues

  • I renamed "locale" -> "language" since a locale is something like "fr-FR" and a language "fr", and we use "fr" as value for the language ; but i can revert these changes and bring back the "locale"
  • Cannot use @Input (getter) to uniformize the code, because the getter is called at any change (including mouse move), then involving too many actions and crash the application
  • (Edit: removed) Could we also remove this button ? @aiAdrian or is it still used for demo purpose on your side ?
    image

Checklist

  • This PR contains a description of the changes I'm making
  • I've read the Contribution Guidelines
  • I've added tests for changes or features I've introduced
  • I documented any high-level concepts I'm introducing in documentation/
  • CI is currently green and this is ready for review

@louisgreiner louisgreiner changed the title Lgr/initialize with navigator language Refacto language + add navigator preferences Nov 13, 2024
@louisgreiner louisgreiner self-assigned this Nov 13, 2024
@louisgreiner louisgreiner force-pushed the lgr/initialize-with-navigator-language branch from 82338e3 to 5e48450 Compare November 13, 2024 14:53
@louisgreiner louisgreiner mentioned this pull request Nov 13, 2024
Open
4 tasks
aiAdrian
aiAdrian approved these changes Nov 13, 2024
View reviewed changes
Copy link
Collaborator

@aiAdrian aiAdrian left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Remove :

image

Removing the switch would be fine for standalone application. Up to you - if there remove ends in a benefit then just remove it. Otherwise fine from ourside

Don't remove :

image

This language switch should stay there because the user might want to switch manually the language preference - especially he works for a company with fixed language covernance which is represented in the IT system. For example, if we have a business system installation, it might depend on the system administrator's settings. For instance, we have German installations, but users might prefer to have another language.

Is this as well fine for you?

Uriel-Sautron
Uriel-Sautron approved these changes Nov 14, 2024
View reviewed changes
Copy link
Collaborator

@Uriel-Sautron Uriel-Sautron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested good work.

emersion
emersion reviewed Nov 14, 2024
View reviewed changes
src/app/app.component.ts Outdated Show resolved Hide resolved
emersion
emersion reviewed Nov 14, 2024
View reviewed changes
src/app/app.component.ts Outdated Show resolved Hide resolved
emersion
emersion reviewed Nov 14, 2024
View reviewed changes
src/app/app.component.ts Outdated Show resolved Hide resolved
@louisgreiner louisgreiner force-pushed the lgr/initialize-with-navigator-language branch from 5fc5974 to b993678 Compare November 14, 2024 11:22
@emersion emersion self-requested a review November 15, 2024 12:20
emersion
emersion reviewed Nov 15, 2024
View reviewed changes
Copy link
Collaborator

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from these two minor issues, looks good to me, thanks for refactoring!

@louisgreiner
refacto localStorage logic
a2e1541 
Signed-off-by: Louis Greiner <[email protected]>
@louisgreiner louisgreiner force-pushed the lgr/initialize-with-navigator-language branch from 8b7e664 to a2e1541 Compare November 15, 2024 14:24
emersion
emersion approved these changes Nov 15, 2024
View reviewed changes
Copy link
Collaborator

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@louisgreiner louisgreiner force-pushed the lgr/initialize-with-navigator-language branch from 41b735c to 2097387 Compare November 15, 2024 14:51
@louisgreiner louisgreiner merged commit 338b69f into main Nov 15, 2024
7 checks passed
@louisgreiner louisgreiner deleted the lgr/initialize-with-navigator-language branch November 15, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Uriel-Sautron Uriel-Sautron Uriel-Sautron approved these changes

@aiAdrian aiAdrian aiAdrian approved these changes

@emersion emersion emersion approved these changes

Assignees

@louisgreiner louisgreiner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@louisgreiner @emersion @aiAdrian @Uriel-Sautron